Interesting one. Suppose there is no value of tagging a js_binary as testonly. I'm still confused how you get this to fail since neither bazel test or bazel coverage should run a js_binary target as test right?

Can you show an example that fails without the change?

We have a js_binary target that is not a test itself, but a dependency of a test. The testonly here is a guard against accidental usage outside of a test dag.

Presumably you could fix this more properly by having js_test also have a _is_test attr that is set to true. This would allow you to ensure all js_tests have a lcov_merger but allow a js_binary to be used with testonly = True.

Can that be shown with a test that fails without this change though?

@jbedard here is a screenshot from me reproducing the failure with bazel coverage using vscode codespaces off of main

Here is the code

# A simple program that runs the Acorn JS parser to produce an AST
js_binary(
    name = "my_bin",
    # Reference the location where the acorn npm module was linked in the root Bazel package
    data = ["//:node_modules/acorn"],
    entry_point = "require_acorn.js",
    testonly = True
)

js_test(name = "test_testonly_bin", data = ["my_bin"], entry_point = "npm_version_test.js")

I can confirm also that applying this fix with that code fixes the bug

i believe this is still wrong as _lcov_merger should be present if the coverage is enabled but since we are sharing implementation between js_test and js_binary we need a sentinel attribute for determining where the impl is being invoked so we can still fail if it's js_test.

@thesayyn so are you suggesting that @joeljeske's fix would be preferred?

Presumably you could fix this more properly by having js_test also have a _is_test attr that is set to true. This would allow you to ensure all js_tests have a lcov_merger but allow a js_binary to be used with testonly = True.

#1486 (comment)

Exactly.

@thesayyn Ok I believe I have added the fix as interpreted from @joeljeske's comment. I confirmed with the same test as before that this fix works too

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

Looks like this breaks some CI stuff. I'll have to spin up an ubuntu container and debug why bazel coverage //... is failing